Convert to keyword arguments#605
Conversation
|
Hmm, I'm seeing some sporadic test failures that I don't think are related to my work (though they could be. The |
|
Interesting, travis is failing for ruby 2.0 because it doesn't seem to be respecting the kwargs in one of the files. Not sure what's going on there, I'll get ruby2.0.0 running locally and dive into it this evening. |
|
I guess this was my lack of knowledge of the keyword arguments feature evolution in ruby. Apparently in 2.0 it raises a syntax error for method definitions like I have updated the few method definitions that were failing with explicit defaults. I have made those defaults I can write a few tests for that behavior as well. |
This is needed for Ruby 2.0.0 because it does not allow keyword args with no default
|
Tests added, and build is green. Let me know what you think @stympy. |
|
I added to the 2.0 milestone since these are BC-breaking changes. |
a359def to
a5d7731
Compare
|
Since we're migrating the objects to namespaces #1318, we'll change the arguments to keywords while adding the namespaces. I think it'd be a better idea to concentrate all the changes in this transition instead of doing things separately. Thanks for opening this PR and suggesting this change. |
|
@vbrazo That makes sense, although looking through some of the comments and PRs linked on #1318 I am not seeing keyword arguments implemented, or even any mention of that they should be. We'll definitely want to make sure contributors are aware of that plan so they can implement their PRs accordingly. |
|
@supremebeing7 you're talking about the lorem PR? I'm still working on this PR, and I'll include the keyword arguments :) I'll make sure to ask contributors to add the keyword arguments. |
|
I just checked our master branch out and we only have a few namespaces and their methods don't have parameters. |
|
Sounds good. Thanks for all your hard work. |
This would resolve #603.
The only two I didn't convert to kwargs was
Internet.fix_umlautsandLorem.charactersbecause those both have a single argument that seemed obvious.I also didn't touch
Financebecause there was no documentation or tests for it, so wasn't sure if it is even being used. I can open an issue for that if you want that added. Could probably throw some time towards that next week.